Skip to content

Fourier Transforms - #1403

Merged
pgrete merged 276 commits into
developfrom
lkasselm/fft
Jul 29, 2026
Merged

Fourier Transforms#1403
pgrete merged 276 commits into
developfrom
lkasselm/fft

Conversation

@lkasselm

@lkasselm lkasselm commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

PR Summary

This PR adds infrastructure for performing distributed Fast Fourier Transforms (FFTs) on uniform meshes in Parthenon. It introduces two new utility classes, a generic spectral output type, and a self-contained example demonstrating correct usage.

Most of this PR (including the documentation) was created in collaboration with LLM chatbots. Claude code refactored the spectrum calculation into its own utility function and created the regression test.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • Any contribution that was created or modified with the assistance of generative AI is disclosed here and in code following the guidelines
  • (@lanl.gov employees) Update copyright on changed files

pgrete and others added 30 commits April 15, 2024 15:50
@pgrete
pgrete changed the base branch from develop to pgrete/single-prec-compile July 10, 2026 17:36
@pgrete
pgrete requested a review from Yurlungur July 10, 2026 17:36
@pgrete

pgrete commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

I just pushed some changes to the PR, in particular

  • make the machinery work with Real=float
  • separating the precision in the spectra calculation (so that these are always calculated in SpecReal = double, which is important for the big boxes in single precision output)
  • Fixed compilation/linking when heffte is externally used
  • added test case to the CalcSpectrum function (now within the utils::fft namespace), which implicitly tests the Hermitian prefactor in calculating the power
  • added an interface to directly pass a data pointer (rather than field string) to CalcSpec
  • added a more performance way for FFTs on GPU using heffte options

This PR should go in after #1412 (as it already contains those changes).

@Yurlungur would be great if you could review (briefly again) so that we get the second approval.

@Yurlungur Yurlungur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Minor nitpicks remaining but they're nonblocking

int low[3]; // lower bound in each dimension
int high[3]; // upper bound in each dimension
int size[3]; // size in each dimension: high - low + 1
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we re-use the RegionSize struct?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regionsize currently only holds effectively the size aspect (plus position related Real info), but we need the lower and upper index bounds here.

Comment thread example/CMakeLists.txt Outdated
add_subdirectory(sparse_advection)
if(PARTHENON_ENABLE_FFT)
add_subdirectory(fourier_transform)
endif() No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline

Comment thread src/outputs/outputs.hpp
Comment on lines +122 to +127
class SpectralOutput : public OutputType {
public:
explicit SpectralOutput(const OutputParameters &oparams) : OutputType(oparams) {}
void WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm,
const SignalHandler::OutputSignal signal) override;
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this also be guarded behind the enable FFT macro?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can stay (so that there's no accidental overwrite by another SpectralOutput type)

Comment thread src/outputs/spectrum.cpp
//! \fn void SpectralOutput::WriteOutputFile()
// \brief Writes a spectrum output file

void SpectralOutput::WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above question about macro

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be safe as the entire file is only selectively included through the macro in CMakeLists.txt

Comment thread src/outputs/spectrum.cpp
Comment on lines +59 to +60
fout << std::format("{:d} {:.15e} {:.15e} {:.15e}\n", i, spectra_h(i, 0),
spectra_h(i, 1), spectra_h(i, 2));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice

Base automatically changed from pgrete/single-prec-compile to develop July 16, 2026 13:08
@pgrete
pgrete enabled auto-merge (squash) July 16, 2026 13:51
@pgrete

pgrete commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Current CI image doesn't ship rocfft (required by heffte).
I fixed this in #1414, which now needs to go in before we can merge this PR.

@pgrete

pgrete commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

@lkasselm the fail in the CUDA test seems legit:

[21](https://github.com/parthenon-hpc-lab/parthenon/actions/runs/29504003750/job/87639925037?pr=1403#step:9:222)
Initialized UniformGridHelper
Mesh layout:
Rank 0 local mesh box: low = (0, 0, 0), high = (15, 15, 15), size = (16, 16, 16)
*** The MPI_Comm_rank() function was called before MPI_INIT was invoked.
*** This is disallowed by the MPI standard.
*** Your MPI job will now abort.
[67e8270543fe:30505] Local abort before MPI_INIT completed completed successfully, but am not able to aggregate error messages, and not able to guarantee that all other processes were killed!

see https://github.com/parthenon-hpc-lab/parthenon/actions/runs/29504003750/job/87639925037?pr=1403

Do you want to take a look at this?
We might need to check that MPI (communicators) are properly initialized (in proper order) and, similarly, also closed cleanly on termination.

@pgrete

pgrete commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

@lkasselm the fail in the CUDA test seems legit:

[21](https://github.com/parthenon-hpc-lab/parthenon/actions/runs/29504003750/job/87639925037?pr=1403#step:9:222)
Initialized UniformGridHelper
Mesh layout:
Rank 0 local mesh box: low = (0, 0, 0), high = (15, 15, 15), size = (16, 16, 16)
*** The MPI_Comm_rank() function was called before MPI_INIT was invoked.
*** This is disallowed by the MPI standard.
*** Your MPI job will now abort.
[67e8270543fe:30505] Local abort before MPI_INIT completed completed successfully, but am not able to aggregate error messages, and not able to guarantee that all other processes were killed!

see https://github.com/parthenon-hpc-lab/parthenon/actions/runs/29504003750/job/87639925037?pr=1403

Do you want to take a look at this? We might need to check that MPI (communicators) are properly initialized (in proper order) and, similarly, also closed cleanly on termination.

Argh, it's the serial test that fails, so no MPI...
I'll fix this.

@pgrete pgrete changed the title Fourier Transforms WIP Fourier Transforms Jul 29, 2026
@pgrete pgrete changed the title WIP Fourier Transforms Fourier Transforms Jul 29, 2026
@pgrete
pgrete disabled auto-merge July 29, 2026 08:10
@pgrete
pgrete enabled auto-merge (squash) July 29, 2026 08:11
@pgrete
pgrete merged commit d9d4af6 into develop Jul 29, 2026
37 of 38 checks passed
@pgrete
pgrete deleted the lkasselm/fft branch July 29, 2026 13:26
@pgrete

pgrete commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Wait what?!
How did this merge go through with one test failing?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants